fix: recover broken publish paths and stuck reconnects - #2030
Open
lukasIO wants to merge 12 commits into
Open
Conversation
Adds several connection-recovery improvements aimed at the class of failures where publishing is broken even though the peer connection still reports connected: - Act on local `ConnectionQuality.Lost`: when the server reports it isn't receiving our media for a sustained period while connected and publishing, force a full reconnect. - Verify the ICE restart actually landed during a resume (wait for `restartingIce` to clear via a matching-offerId answer) instead of only waiting for `connected`, and escalate a rejected publisher answer during a resume to a full reconnect. - Recreate the peer connection (via escalation) when an ICE restart is needed but there is no remote description to restart on, rather than stalling on `renegotiate`. - Add outbound-RTP liveness to `verifyTransport()` (bytesSent must advance with active senders) and bound how long a transport may stay CONNECTING. - Preserve a full-reconnect request that arrives mid-resume so a successful resume no longer clears it. - On a detected connection state mismatch, attempt a full reconnect (keeping the room alive) instead of tearing the session down. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 3e59e3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
size-limit report 📦
|
Reverts the resumeConnection change that blocked the resume until the publisher's ICE restart offer was answered (`restartingIce` cleared). When only the signal blips but the media path is fine, the server may never answer the resume's ICE-restart offer, so gating on it hangs the resume and it never emits `resumed` (regressed the signalDisconnectDuringResume e2e test). A stale-but-connected publisher is instead caught at runtime by the local `ConnectionQuality.Lost` handler and the outbound-RTP liveness check in `verifyTransport()`, without breaking resume semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lukasIO
marked this pull request as ready for review
August 3, 2026 15:19
1egoman
approved these changes
Aug 3, 2026
1egoman
left a comment
Contributor
There was a problem hiding this comment.
I think ideally it would be good to get some test coverage for these paths - I know that's challenging since this is fairly intermixed in the existing room connection logic. Maybe some e2e test(s)?
The bytesSent-advancing heuristic false-positives on legitimate custom tracks that don't emit media continuously (static screen share, on-demand canvas, silent/push-to-talk audio), triggering unnecessary reconnects. Remove it and revert verifyTransport to a synchronous check. The "stop treating an indefinitely-connecting transport as healthy" bound is kept — it's about connection state, not media, so it doesn't share the false-positive concern. Detection of a genuinely broken publish path is left to the server-driven local ConnectionQuality.Lost handling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unit tests for the engine's local Lost-quality handling: a sustained local LOST while connected and publishing forces a full reconnect, recovery cancels the pending trigger, and it stays put when not publishing, not connected, or the LOST is for another participant. A genuine server LOST can't be produced from a browser page (a live sender keeps RTCP flowing), so this replaces the grey-box e2e attempt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Unit tests for the attemptReconnect finally-dispatch (Fix 5): a full reconnect requested while a resume is in flight survives the successful resume and is dispatched afterwards; an ordinary successful resume does not re-dispatch; a successful full reconnect clears the flag; and the failure path is not double-dispatched. Replaces the grey-box fullReconnectDuringResume e2e test, whose triggering leave was injected client-side anyway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nsition verifyTransport() owned transportConnectingSince but leaked it on several exit paths (no pcManager, ws closed, the stuck verdict) and across cleanupPeerConnections(), so a timestamp captured before a failure survived the teardown/rebuild. Since Room pauses the reconcile during Resuming/Restarting and resumes it only after recovery, the first post-recovery tick that saw CONNECTING measured against the ancient timestamp and instantly reported the transport as stuck, causing a spurious extra reconnect. Record the entry time in the pcManager state-change handler (which only fires on real transitions, overwriting on each entry into CONNECTING) and make verifyTransport() a pure read that fails open when no timestamp is recorded. Also clear it in cleanupPeerConnections() for the paths where onStateChange is detached before teardown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
scheduleLostQualityReconnect's countdown was only cancelled by a non-LOST quality update or close(), not when a reconnect starts. A countdown armed from a previous session's LOST verdict could fire shortly after a fast reconnect completed and force another full reconnect, before the server had evaluated the new session. Clear it at the start of attemptReconnect so the countdown always reflects the current session. attemptReconnect (rather than cleanupPeerConnections) is the single entry point that also covers the resume path, which keeps its peer connections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…onnect attemptReconnect only preserved a full-reconnect request that arrived during a resume. When the attempt itself was a full reconnect, the success path unconditionally cleared fullReconnectOnNext, discarding a request (e.g. a server RECONNECT leave) that arrived while restartConnection() was running — its zero-delay reconnect was a no-op under the attemptingReconnect guard and then wiped by clearPendingReconnect(). Consume the flag at the start of the attempt (read into a local, reset to false) so any true value seen afterwards is unambiguously a new request, handled by the existing finally dispatch for both paths. The catch still escalates a failed full reconnect back to a full reconnect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
A set of connection-recovery improvements targeting the class of failures where publishing is broken even though the peer connection still reports
connected— cases the current logic doesn't notice, so the client sits on a dead session without reconnecting.Changes
Act on local
ConnectionQuality.Lost(RTCEngine) —LOSTis the server's verdict that it isn't receiving our media. If it staysLOSTfor a sustained window (5s) while connected and actively publishing, force a full reconnect. Catches the whole class regardless of root cause, and is server-authoritative. Any non-LOSTupdate cancels the pending trigger.Recreate the PC when an ICE restart has no remote description (
PCTransport) — the old TODO fell through torenegotiate = true, which stalls (the pending offer is never answered). Now it throws so the caller escalates to a full reconnect (which rebuilds the peer connections). Only reachable on the resume path.Bound how long a transport may sit in
CONNECTING(RTCEngine.verifyTransport) — a transport stuck inCONNECTINGnever reachesCONNECTEDnor reportsFAILED, so it was treated as healthy indefinitely; it's now bounded bypeerConnectionTimeout.Don't clobber a full-reconnect request that arrives mid-resume (
RTCEngine) — a successful resume no longer clearsfullReconnectOnNextit didn't act on (e.g. a serverRECONNECTleave that arrived during the resume); it's dispatched after the attempt settles.Reconnect on a detected state mismatch instead of tearing down (
Room,RTCEngine) — when the connection-reconcile safety net trips (transport silently died while we looked connected), it now triggers a full reconnect via the existing engine (reusing saved credentials, keeping the room alive) rather than terminally disconnecting. Terminal teardown remains the fallback when there's no usable engine or the reconnect ultimately fails.Reviewer notes
DisconnectReason.STATE_MISMATCH) previously emittedRoomEvent.Disconnectedimmediately. It now goesReconnecting → (Reconnected | Disconnected), matching how every other transport failure is handled. Apps that watched for an immediateSTATE_MISMATCHdisconnect to trigger their own reconnect will see the reconnecting flow instead.verifyTransport()stays synchronous (an earlier revision briefly made it async for the stall check; that's reverted).Testing
pnpm type:check,pnpm lint(0 errors in changed files),pnpm throws:check, andpnpm test(663 tests) all pass. New e2e coverage (in the e2e repo):connectionQualityLostReconnect(real serverLOSTvia a published-but-silent track → full reconnect) andfullReconnectDuringResume(mid-resumeRECONNECTleave still triggers a full reconnect).🤖 Generated with Claude Code